Repository navigation
Conversation
…rd I2S DAC truths Reverse-engineered from the tig/xuss-c first ship. The pattern across every finding was the same: the plate is the spec the agent actually follows. - Link surface: the plate required `repl` / `reboot` in prose but shipped only `identity` in code, so the product inherited the code and shipped without an escape hatch. Parsing/dispatch moves to src/domain.c (gcu_parse_command / gcu_handle_command) with park_outputs + reboot HAL hooks; main.c now only moves bytes. `repl` parks outputs and stops the tick driving them; `reboot` defers the reset so the ack flushes first; unknown input fails closed. - Host coverage: dispatch in firmware/main.c was device-only code, so "protocol parsing" could never be part of a host-green claim. host/test_protocol.c covers identity, repl parking, deferred reboot, blank lines, whole-token matching, and undersized buffers. Third test file exists partly so the plate reads as a floor rather than a ceiling — the product shipped exactly the two test files the plate happened to ship. - Deploy assets: [[deploy.data]] has existed since #79 but appeared nowhere in this plate, so the product hand-rolled the esptool write_flash wall the feature was built to delete. Commented example in silico.toml plus a note in install/README.md. - knowledge/esp32-audio.md: the C section presented dac_continuous as the path, which is measurably wrong on M5GO (descriptor timeouts under load). Adds the I2S -> built-in DAC recipe and the ONLY_RIGHT 2x-fast trap that mimics a sample-rate bug, plus amp gain ceiling and the park-at-mid-then-zero click. - AGENTS.md: product-path proves defaults are loaded, not that normative behavior is covered; display HAL granularity (compose-and-blit, not per-pixel fill_rect text). Host gate green: 3/3 ctest on the plate, full silico pytest suite passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDzUSCHwM6cs47eHGXX5y9
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f1550f405
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| char id[64]; | ||
| gcu_identity_line(id, (int)sizeof id); | ||
| printf("%s\n", id); | ||
| if (gcu_handle_command(st, line, reply, (int)sizeof reply)) { |
There was a problem hiding this comment.
Discard the remainder of an overlong serial line
When a serial line exceeds 47 bytes, the existing overflow branch resets n to zero but continues collecting the suffix, and this new dispatch call treats that suffix as a complete command. Thus an oversized or malformed line ending in repl or reboot can unexpectedly park the product or reset it instead of failing closed; retain an overflow flag and ignore all bytes until the next delimiter.
AGENTS.md reference: silico/plates/gcu-c/AGENTS.md:L105-L110
Useful? React with 👍 / 👎.
| st.reboot_pending = 0; | ||
| if (hal && hal->reboot) { | ||
| hal->reboot(hal); | ||
| } |
There was a problem hiding this comment.
Refuse reboot when the HAL cannot reset
When a derived product leaves the explicitly optional hal->reboot hook as NULL, the command has already replied ok reboot and parked its outputs, but this block clears reboot_pending and performs no reset. The device then remains parked indefinitely while the host believes it rebooted, defeating the escape hatch; reject reboot before acknowledging it when the hook is unavailable.
AGENTS.md reference: silico/plates/gcu-c/AGENTS.md:L99-L101
Useful? React with 👍 / 👎.
Reverse-engineered from reviewing the tig/xuss-c first ship against its
spec.md. Five unrelated findings had one mechanism behind them: agents extend what the plate ships and ignore what the plate only says.Closes nothing outright; advances #111, #112, #113, #114.
The escape hatch now exists in code, not just in prose
The plate's
AGENTS.mdsaid "Escape hatch (repl/reboot) is a product requirement." The plate'smain.cimplementedidentityonly. xuss-c inherited the code — same function name, same shape — and shipped without a door, which its own spec says fails L1.Compounding it,
silico inspectknocksidentityand nothing else, so the one command silico verifies is the one that got built.gcu_parse_command/gcu_handle_commandinsrc/domain.creplparks outputs and setsparked;gcu_tickthen stops driving the face until next bootrebootparks, acks, and setsreboot_pending—main.cflushes the reply then callshal->reboot, so the host sees the ackerr, not a help essaypark_outputsandreboot;hal_board.cimplements bothParsing moved across the HAL seam
Dispatch used to live in
firmware/main/main.c— device-side, outside the host build. That made spec-layer L0's "protocol parsing" requirement structurally impossible to satisfy, on any product using this plate.main.cnow only moves bytes.A third test file
host/test_protocol.ccovers identity,replparking (including that ticks stay parked afterward), deferredreboot, whole-token matching (identityXandrepmust not match), blank lines, and an undersized reply buffer being refused rather than overflowed.It exists partly because three files read differently than two: xuss-c shipped exactly the two test files the plate shipped, and its spec's normative tables — theme cycle order, song state machine, button map — got none. The plate should read as a floor.
[[deploy.data]]is discoverable nowThe feature has existed since #79, with tests, and its docstring says it exists so there is "no free-form esptool essay." It appeared nowhere in this plate. xuss-c consequently hand-rolled a two-command
gen_spiffs_image.py+esptool write_flashwall into its operator docs — and because the asset ships out-of-band, its firmware grew a fallback that plays the boot riff when the track is missing, where the spec requires a clear refusal.Commented block in
silico.toml, plus aninstall/README.mdnote about not pasting rawwrite_flashinto operator docs.knowledge/esp32-audio.md— the compounding missThe C section presented
dac_continuousas the native path. On M5GO that is measurably wrong: descriptor timeouts under load on multi-minute streams. Six audio commits of thrash in xuss-c produced durable board truths that were never written back, so the next agent would have repeated them.Added, with the two paths now presented as a board-dependent choice rather than one answer:
RIGHT_LEFT,BOTH_EN, 8×256 DMA, APLL)I2S_CHANNEL_FMT_ONLY_RIGHTclocks content ~2× fast and the symptom mimics a sample-rate bug — the natural wrong fix is halving the rate. Suspect channel format before touching the asset rate.AGENTS.md
product-pathproves defaults are loaded, not that normative behavior is covered — different claims. Plus display HAL granularity: compose a region and blit once; a 5×7 glyph drawn per-pixel is ~35 SPI transactions per character, and a six-row readout at 10 Hz becomes tens of thousands per second, which is what fights an audio DMA feeder and reaches the operator as stutter.Verification
Also compiled clean under
-Wall -Wextra -Wpedantic -std=c11.Scope
Plate and knowledge only — no CLI behavior change. The real fix for the inspect gap is
[protocol].requiredinsilico.tomlsoinspectexercises a declared surface instead of justidentity; that is left open in #111 because it needs a maintainer call on warn-vs-fail.🤖 Generated with Claude Code
https://claude.ai/code/session_01PDzUSCHwM6cs47eHGXX5y9
Generated by Claude Code